-
Notifications
You must be signed in to change notification settings - Fork 270
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Brian Chow] iP #296
base: master
Are you sure you want to change the base?
[Brian Chow] iP #296
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found the code easy to read and follow. There are a couple of coding standard violations. The InputHandler class is also very long and could be shortened with the use of more classes to split up the code in the switch-case and if-else.
src/main/java/Task.java
Outdated
* Represents a Task. Contains a Task constructor, two methods to mark and unmark tasks, toString() method as well as a isMark() method to check if Task is marked | ||
*/ | ||
public class Task { | ||
private boolean mark; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I like the naming of the boolean variable. As much as possible, use a prefix such as is, has, was, etc. for boolean variable/method names so that linters can automatically verify that this style rule is being followed. Perhaps it would be better to name it as follows?
private boolean mark; | |
private boolean isMarked; |
src/main/java/Task.java
Outdated
/** | ||
* markTask as done | ||
*/ | ||
public void markTask () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be named in this manner? Perhaps the following would comply with the coding standards better.
public void markTask () { | |
public void setMarkedTask () { |
src/main/java/Task.java
Outdated
/** | ||
* unmarkTask | ||
*/ | ||
public void unmarkTask() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be named in this manner? Perhaps the following would comply with the coding standards better.
public void unmarkTask() { | |
public void setUnmarkedTask () { |
src/main/java/Task.java
Outdated
* | ||
* @return boolean on whether task is marked | ||
*/ | ||
public boolean isMark() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be changed to another name, if you do decide to accept the suggestion of the boolean variable naming? Perhaps the following would work.
public boolean isMark() { | |
public void hasBeenMarked () { |
src/main/java/InputHandler.java
Outdated
String[] splitInput = input.split(" "); | ||
String inputCommand = splitInput[0]; | ||
switch (inputCommand) { | ||
case "todo": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't like the indentation after the switch, are you sure this complies with the coding standard? Consider leaving no indentation for the case statements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty neat overall, LGTM! Just need to clean up a little on the javadocs and abstracting out certain logic to make the code a little bit better to read.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty neat overall, LGTM! Just need to clean up a little on the javadocs and abstracting out certain logic to make the code a little bit better to read.
src/main/java/Deadline.java
Outdated
*/ | ||
public class Deadline extends Task { | ||
private String dueDate; | ||
public Deadline(String name, String time) {super(name); this.dueDate = time;} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing javadocs for constructor.
src/main/java/Deadline.java
Outdated
/** | ||
* @override | ||
* @return String of Deadline task, eg [D][X] Deadline (by:XX) | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Override should not be in the javadoc. Missing method's description for the javadoc.
Recommendation:
/**
* Prints the String representation for the Deadline task.
*
* @return String of Deadline task, eg [D][X] Deadline (by:XX)
* /
@Override
public String toString() {...}
src/main/java/DukeException.java
Outdated
@@ -0,0 +1,12 @@ | |||
public class DukeException extends Exception{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing javadoc for class.
src/main/java/InputHandler.java
Outdated
* Processes the input into 7 categories: Todo, Event, Deadline, list, mark, unmark, bye and throws error | ||
*/ | ||
public class InputHandler { | ||
private ArrayList<Task> arr; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be better to have a more specific and plural name for a list of task.
Recommendation:
private ArrayList<Task> tasks;
src/main/java/InputHandler.java
Outdated
|
||
|
||
/** | ||
* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing method description for javadoc.
src/main/java/InputHandler.java
Outdated
public boolean handleInput(String input) throws DukeException { | ||
String[] splitInput = input.split(" "); | ||
String inputCommand = splitInput[0]; | ||
switch (inputCommand) { | ||
case "todo": | ||
if (splitInput.length > 1) { | ||
String[] nameArray = Arrays.copyOfRange(splitInput, 1, splitInput.length); | ||
String name = String.join(" ", nameArray); | ||
Todo newTodo = new Todo(name); | ||
arr.add(newTodo); | ||
printAddTaskMessage(newTodo); | ||
return false; | ||
} else { | ||
throw new DukeException(":( OOPS!!! The description of a todo cannot be empty. Correct usage: todo [task]"); | ||
} | ||
case "event": | ||
if (splitInput.length > 3) { | ||
String[] stringArrayExcludingEvent = Arrays.copyOfRange(splitInput, 1, splitInput.length); | ||
String stringExcludingEvent = String.join(" ", stringArrayExcludingEvent); | ||
String[] nameAndTimeArray = stringExcludingEvent.split("/at"); | ||
String name = nameAndTimeArray[0]; | ||
String time = nameAndTimeArray[1]; | ||
Event newEvent = new Event(name, time); | ||
arr.add(newEvent); | ||
printAddTaskMessage(newEvent); | ||
return false; | ||
} else { | ||
throw new DukeException(":( OOPS!!! The description of a event cannot be empty. Correct usage: event [task] /at [time]"); | ||
} | ||
case "deadline": | ||
if (splitInput.length > 3) { | ||
String[] stringArrayExcludingDeadline = Arrays.copyOfRange(splitInput, 1, splitInput.length); | ||
String stringExcludingDeadline = String.join(" ", stringArrayExcludingDeadline); | ||
String[] nameAndTimeArray = stringExcludingDeadline.split("/by"); | ||
String name = nameAndTimeArray[0]; | ||
String time = nameAndTimeArray[1]; | ||
Deadline newDeadline = new Deadline(name, time); | ||
arr.add(newDeadline); | ||
printAddTaskMessage(newDeadline); | ||
return false; | ||
} else { | ||
throw new DukeException(":( OOPS!!! The description of a deadline cannot be empty. Correct usage: deadline [task] /by [time]"); | ||
} | ||
case "list": | ||
if (splitInput.length == 1) { | ||
System.out.println("Here are the tasks in your list:"); | ||
int i = 0; | ||
for (Task item : arr) { | ||
i += 1; | ||
if (item.isMark()) { | ||
System.out.println(i + ". " + item); | ||
} else { | ||
System.out.println(i + ". " + item); | ||
} | ||
} | ||
return false; | ||
} else { | ||
throw new DukeException("Wrong usage of list! Correct usage: list"); | ||
} | ||
case "mark": | ||
if (splitInput.length == 2) { | ||
System.out.println("Nice! I've marked this task as done:\n"); | ||
int idx = Integer.parseInt(splitInput[1]) - 1; | ||
Task taskToBeMarked = arr.get(idx); | ||
taskToBeMarked.markTask(); | ||
return false; | ||
} else { | ||
throw new DukeException("Wrong usage of mark! Correct usage: mark [index]"); | ||
} | ||
case "unmark": | ||
if (splitInput.length == 2) { | ||
System.out.println("OK, I've marked this task as not done yet:\n"); | ||
int idx = Integer.parseInt(splitInput[1]) - 1; | ||
Task taskToBeUnmarked = arr.get(idx); | ||
taskToBeUnmarked.unmarkTask(); | ||
return false; | ||
} else { | ||
throw new DukeException("Wrong usage of unmark! Correct usage: unmark [index]"); | ||
} | ||
case "delete": | ||
if (splitInput.length == 2) { | ||
int idx = Integer.parseInt(splitInput[1]) - 1; | ||
Task taskToBeDeleted = arr.get(idx); | ||
arr.remove(idx); | ||
System.out.println("Noted. I've removed this task:\n" + taskToBeDeleted + "\nNow you have " + arr.size() + " tasks in the list"); | ||
return false; | ||
} else { | ||
throw new DukeException("Wrong usage of delete! Correct usage: delete [index]"); | ||
} | ||
case "bye": | ||
return true; | ||
default: | ||
throw new DukeException(":( OOPS!!! I'm sorry, but I don't know what that means! Possible commands: todo [task], event [task] /at [time]," | ||
+ " deadline [task] /by [time], mark [index], unmark [index], delete [index], bye"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method severely exceeds the recommended number of lines in a method (30).
Recommendation:
Can try to abstract out the details of each case statement into a different method.
src/main/java/Task.java
Outdated
public String name; | ||
|
||
/** | ||
* Constructor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be better to have a more specific description of a constructor instead of simply "Constructor".
Recommendation:
/**
* Creates a task object with the given parameter as the name of the task.
* @param name name of the task
*/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, code was easy to read. Just minor coding standard violations to take down note of. Good job.
src/main/java/Deadline.java
Outdated
private String dueDate; | ||
public Deadline(String name, String time) {super(name); this.dueDate = time;} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could leave spacing for better readability.
src/main/java/Duke.java
Outdated
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could clear by these empty spaces.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed
src/main/java/Event.java
Outdated
*/ | ||
public class Event extends Task { | ||
private String dueDate; | ||
public Event (String name, String time) { super(name); this.dueDate = time;} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This do not follow the module coding standards.
src/main/java/InputHandler.java
Outdated
case "bye": | ||
return true; | ||
default: | ||
throw new DukeException(":( OOPS!!! I'm sorry, but I don't know what that means! Possible commands: todo [task], event [task] /at [time]," |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could add indentation.
src/main/java/Task.java
Outdated
import java.util.Arrays; | ||
|
||
/** | ||
* Represents a Task. Contains a Task constructor, two methods to mark and unmark tasks, toString() method as well as a isMark() method to check if Task is marked |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could add indentation.
… tasks with methods and Parser makes sense of user input for Duke
…ot in correct format. Added proper javadocs documentation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Neat and organised codebase, variables are well named. Good job 👍
src/main/java/Deadline.java
Outdated
/** | ||
* @override | ||
* @return String of Deadline task, eg [D][X] Deadline (by:XX) vs [D][✓] Deadline (by;XX) | ||
*/ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps you might want to place the JavaDoc directly above the method without a spacing, as well as move the override outside for it to be in effect :)
src/main/java/Duke.java
Outdated
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed
src/main/java/Event.java
Outdated
public Event (String name, String date) throws DateTimeParseException { | ||
super(name); | ||
this.dueDate = LocalDate.parse(date); | ||
this.dueTime = null; | ||
} | ||
public Event (String name, String date, String time) throws DateTimeParseException { | ||
super(name); | ||
this.dueDate = LocalDate.parse(date); | ||
this.dueTime = LocalTime.parse(time); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might want to include a line of space between the attributes and between methods :)
src/main/java/InputHandler.java
Outdated
switch (inputCommand) { | ||
case "todo": | ||
if (splitInput.length > 1) { | ||
Todo newTodo = (Todo) parser.parse(CommandType.TODO, splitInput); | ||
this.storage.writeData(newTodo); | ||
printAddTaskMessage(newTodo); | ||
return false; | ||
} else { | ||
throw new DukeException(":( OOPS!!! The description of a todo cannot be empty. Correct usage: todo [task]"); | ||
} | ||
case "event": | ||
if (splitInput.length > 3) { | ||
Event newEvent = (Event) parser.parse(CommandType.EVENT, splitInput); | ||
this.storage.writeData(newEvent); | ||
printAddTaskMessage(newEvent); | ||
return false; | ||
|
||
} else { | ||
throw new DukeException(":( OOPS!!! The description of a event cannot be empty. Correct usage: event [task] /at [time]"); | ||
} | ||
case "deadline": | ||
if (splitInput.length > 3) { | ||
Deadline newDeadline = (Deadline) parser.parse(CommandType.DEADLINE, splitInput); | ||
this.storage.writeData(newDeadline); | ||
printAddTaskMessage(newDeadline); | ||
return false; | ||
} else { | ||
throw new DukeException(":( OOPS!!! The description of a deadline cannot be empty. Correct usage: deadline [task] /by [time]"); | ||
} | ||
case "list": | ||
if (splitInput.length == 1) { | ||
parser.parse(CommandType.LIST, this.storage, splitInput); | ||
return false; | ||
} else { | ||
throw new DukeException("Wrong usage of list! Correct usage: list"); | ||
} | ||
case "mark": | ||
if (splitInput.length == 2) { | ||
parser.parse(CommandType.MARK, this.storage, splitInput); | ||
return false; | ||
} else { | ||
throw new DukeException("Wrong usage of mark! Correct usage: mark [index]"); | ||
} | ||
case "unmark": | ||
if (splitInput.length == 2) { | ||
parser.parse(CommandType.UNMARK, this.storage, splitInput); | ||
return false; | ||
} else { | ||
throw new DukeException("Wrong usage of unmark! Correct usage: unmark [index]"); | ||
} | ||
case "delete": | ||
if (splitInput.length == 2) { | ||
parser.parse(CommandType.DELETE, this.storage, splitInput); | ||
return false; | ||
} else { | ||
throw new DukeException("Wrong usage of delete! Correct usage: delete [index]"); | ||
} | ||
case "bye": | ||
return true; | ||
default: | ||
throw new DukeException(":( OOPS!!! I'm sorry, but I don't know what that means! Possible commands: todo [task], event [task] /at [time]," | ||
+ " deadline [task] /by [time], mark [index], unmark [index], delete [index], bye"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good use of switch statement to clean up code. Easy to follow 👍
src/main/java/Parser.java
Outdated
|
||
public class Parser { | ||
|
||
public Task parse(InputHandler.CommandType type, String[] splitInput) throws DukeException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps it may be easier to understand if the 2 parse functions were combined into 1 :)
src/main/java/Storage.java
Outdated
String mark = (task.hasBeenMarked()) ? "[✓]" : "[X]"; | ||
output = "[T] " + mark + " / " + task.name + "\n"; | ||
} else if (task instanceof Deadline deadline) { | ||
String mark = (deadline.hasBeenMarked()) ? "[✓]" : "[X]"; | ||
output = "[D] " + mark + " / " + deadline.name + " / " + deadline.dueDate + " / " + deadline.dueTime + "\n"; | ||
} else if (task instanceof Event event) { | ||
String mark = (event.hasBeenMarked()) ? "[✓]" : "[X]"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ticks are a nice touch 😊👌
src/main/java/Storage.java
Outdated
if (task.hasBeenMarked()) { | ||
listOfTasks += i + ". " + task + "\n"; | ||
} else { | ||
listOfTasks += i + ". " + task + "\n"; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good use of spaces between operators 👍
Add Assertions for index out of bounds exception for delete/mark/unmark commands. Had to handle this exception for user error, change made addresses this. It is done using assertions to ensure index is within bounds. Change some of the code behind the bye command and storage. GUI was not exiting properly when bye was called, and some of the writing to storage had issues. Add a new method rewriteData() to address this issue and fixed bye command. Change storage writing for marked vs unmarked to 1(marked) vs 0(unmarked), since the GUI had issues displaying the tick.
Update and add documentation for all methods. Improve abstraction throughout the code by abstracting out many Strings for return statements. Some methods reused the same String, hence abstracting them out makes the code cleaner. Shorten the method length for methods in Parser.java, InputHandler.java and Storage.java as method lengths were too long and add new methods for each case in the switch to handle the code. This was aimed at making the code cleaner.
Code Quality
Add snooze command for the type B Extension. This functionality was added as an 8th command type.
Duke
Duke is a interactive chatbot with UI.
Duke is a task tracker that can track three categories of tasks:
Setting up
Setting up Duke is extremely easy. All you need to do is:
It's that simple! :)
Commands
There are a few commands you can run in Duke. These include:
Using an IDE
You can further edit/modify Duke by navigating to the
src
folder which contains all thejava
filesYou can run it from Launcher.java by using main: